Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Negative's arbitrary instance work like that of Positive #377

Merged
merged 1 commit into from
Mar 25, 2024
Merged

Make Negative's arbitrary instance work like that of Positive #377

merged 1 commit into from
Mar 25, 2024

Conversation

Wheatwizard
Copy link
Contributor

@Wheatwizard Wheatwizard commented Mar 22, 2024

Currently Negative and Positive have arbitrary instances which are inconsistent with each other.

Positive generates an arbitrary, takes its absolute value, and checks that it is greater than 0; while Negative just checks the value is less than zero.

One would expect that the instance would behave like:

arbitrary = fmap (Negative . negate . getPositive) arbitrary

however, because of the differences in the implementations, it does not.

This MR would make the two have consistent behavior by adjusting Negative's arbitrary instance to have parity with Positive's instance.

The alternative could be done, adjusting Positive's instance to match that of Negative, but I chose to adjust it this way since the definition of arbitrary for Positive rejects fewer values.

@MaximilianAlgehed
Copy link
Collaborator

I like it!

The only shortcoming I see is that this will crash in more cases where either abs or negate aren't defined than the old instance would. I don't think this shortcoming is particularly severe given that you probably shouldn't be using Negative at a type that doesn't implement negate and abs.

@MaximilianAlgehed MaximilianAlgehed merged commit 57c7ddd into nick8325:master Mar 25, 2024
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants